fix: added gcs name validation #10426

nbugden · 2024-04-12T13:47:29Z

Fixes hashicorp/terraform-provider-google#17831

Release Note Template for Downstream PRs (will be copied)

storage: added validation to `name` field in `google_storage_bucket` resource

github-actions · 2024-04-12T13:48:08Z

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@zli82016, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

zli82016 · 2024-04-12T16:26:05Z

/gcbrun

modular-magician · 2024-04-12T16:46:22Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 3 files changed, 80 insertions(+), 4 deletions(-))
google-beta provider: Diff ( 3 files changed, 80 insertions(+), 4 deletions(-))

daniel-cit · 2024-04-12T17:29:46Z

is this the same validation

https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.erb#L581C1-L583C3

that was implented in #6250 ?

nbugden · 2024-04-12T17:44:49Z

@daniel-cit there is some overlap. The differences with #6250 include:

The original validations DON'T run when terraform plan is run. They are only run when attempting to create the bucket as they are part of the resourceStorageBucketCreate function.
The original validations are also incomplete as they do not apply all of the rules from the docs

IMO it would make sense to keep the validations in one place. I tried to follow the convention of the other validations which were in the verify package. I'd be happy to remove the changes from #6250 to deduplicate validations if that's preferred.

modular-magician · 2024-04-12T17:48:34Z

Tests analytics

Total tests: 3572
Passed tests: 3204
Skipped tests: 368
Affected tests: 0

Click here to see the affected service packages

all service packages are affected

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log

nbugden · 2024-04-12T18:13:39Z

@daniel-cit just pushed 8b1cbd0 which removes the CheckGCSName function in favor of the validations proposed in this PR.

daniel-cit · 2024-04-12T18:31:31Z

@daniel-cit just pushed 8b1cbd0 which removes the CheckGCSName function in favor of the validations proposed in this PR.

Maybe it would be good to also cover the same test cases from the old validation code

https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/tpgresource/utils_test.go#L1262C1-L1285C36

to ensure that the changes are compatible.

daniel-cit · 2024-04-12T18:50:32Z

@nbugden
what would happen if the name of the bucket uses a random resource to compose the name like

resource "random_string" "suffix" {
  length  = 64
  numeric = true
  lower   = false
  upper   = false
  special = false
}

resource "google_storage_bucket" "after_plan" {
  name          = "plan-${random_string.suffix.result}"
  location      = "US"
}

the value random_string.suffix.result will only be known at apply(create) time.

in this case, will the resource delay validation to the apply phase ?

Will the new validation be used in all the execution flows: read, create, update, and destroy?

Using the current version of the resource, the execution will fail on the apply phase

Plan: 2 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

random_string.suffix: Creating...
random_string.suffix: Creation complete after 0s [id=9204249628951144225952644330474297575131248816450437079420460938]
google_storage_bucket.after_plan: Creating...
╷
│ Error: error: bucket name validation failed plan-9204249628951144225952644330474297575131248816450437079420460938
│ 
│   with google_storage_bucket.after_plan,
│   on bucket.tf line 9, in resource "google_storage_bucket" "after_plan":
│    9: resource "google_storage_bucket" "after_plan" {
│

however, since the validation is only on the create function for the current resource, it is possible to fix this error reducing the length of the random_string

resource "random_string" "suffix" {
  length  = 34
  numeric = true
  lower   = false
  upper   = false
  special = false
}

  # random_string.suffix must be replaced
-/+ resource "random_string" "suffix" {
      ~ id          = "9204249628951144225952644330474297575131248816450437079420460938" -> (known after apply)
      ~ length      = 64 -> 34 # forces replacement
      ~ result      = "9204249628951144225952644330474297575131248816450437079420460938" -> (known after apply)
        # (9 unchanged attributes hidden)
    }

Plan: 2 to add, 0 to change, 1 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

random_string.suffix: Destroying... [id=9204249628951144225952644330474297575131248816450437079420460938]
random_string.suffix: Destruction complete after 0s
random_string.suffix: Creating...
random_string.suffix: Creation complete after 0s [id=2439292337254968112779191630127567]
google_storage_bucket.after_plan: Creating...
google_storage_bucket.after_plan: Creation complete after 2s [id=plan-2439292337254968112779191630127567]

Apply complete! Resources: 2 added, 0 changed, 1 destroyed.

will this behavior be preserved with the new validation ?

Sorry asking all these questions but I was affect by the initial version of the validation and I would like this change to be a smooth one.

nbugden · 2024-04-12T19:09:22Z

Maybe it would be good to also cover the same test cases from the old validation code
https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/tpgresource/utils_test.go#L1262C1-L1285C36
to ensure that the changes are compatible.

@daniel-cit b3f3d9e adds the tests for parity from CheckGCSName

nbugden · 2024-04-12T19:25:56Z

what would happen if the name of the bucket uses a random resource to compose the name

@daniel-cit in that case since the name cannot be known prior to apply the validation is not run during the plan (see below)

Terraform Config

resource "random_string" "suffix" {
  length  = 64
  numeric = true
  lower   = false
  upper   = false
  special = false
}

resource "google_storage_bucket" "after_plan" {
  name     = "plan-${random_string.suffix.result}"
  location = "US"
}

Terraform Plan

Terraform Command

TF_CLI_CONFIG_FILE="$HOME/terraform/magic-modules-dev-override.tfrc" terraform plan -target='google_storage_bucket.after_plan'

Output

│ Warning: Provider development overrides are in effect
│ 
│ The following provider
│ development overrides are
│ set in the CLI
│ configuration:
│  - hashicorp/google in /Users/nathan.bugden/go/bin
│ 
│ The behavior may
│ therefore not match any
│ released version of the
│ provider and applying
│ changes may cause the
│ state to become
│ incompatible with
│ published releases.
╵

Terraform used the selected
providers to generate the
following execution plan.
Resource actions are
indicated with the
following symbols:
  + create

Terraform will perform the following actions:

  # google_storage_bucket.after_plan will be created
  + resource "google_storage_bucket" "after_plan" {
      + effective_labels            = (known after apply)
      + force_destroy               = false
      + id                          = (known after apply)
      + location                    = "US"
      + name                        = (known after apply)
      + project                     = (known after apply)
      + project_number              = (known after apply)
      + public_access_prevention    = (known after apply)
      + rpo                         = (known after apply)
      + self_link                   = (known after apply)
      + storage_class               = "STANDARD"
      + terraform_labels            = (known after apply)
      + uniform_bucket_level_access = (known after apply)
      + url                         = (known after apply)
    }

  # random_string.suffix will be created
  + resource "random_string" "suffix" {
      + id          = (known after apply)
      + length      = 64
      + lower       = false
      + min_lower   = 0
      + min_numeric = 0
      + min_special = 0
      + min_upper   = 0
      + number      = true
      + numeric     = true
      + result      = (known after apply)
      + special     = false
      + upper       = false
    }

Plan: 2 to add, 0 to change, 0 to destroy.

Terraform Apply

When the same configuration is applied, the validation is run prior to creating the bucket.

Terraform Command

TF_CLI_CONFIG_FILE="$HOME/terraform/magic-modules-dev-override.tfrc" terraform apply -target='google_storage_bucket.after_plan'

Terraform Output

│ Warning: Provider development overrides are in effect
│ 
│ The following provider
│ development overrides are
│ set in the CLI
│ configuration:
│  - hashicorp/google in /Users/nathan.bugden/go/bin
│ 
│ The behavior may
│ therefore not match any
│ released version of the
│ provider and applying
│ changes may cause the
│ state to become
│ incompatible with
│ published releases.
╵

Terraform used the selected
providers to generate the
following execution plan.
Resource actions are
indicated with the
following symbols:
  + create

Terraform will perform the following actions:

  # google_storage_bucket.after_plan will be created
  + resource "google_storage_bucket" "after_plan" {
      + effective_labels            = (known after apply)
      + force_destroy               = false
      + id                          = (known after apply)
      + location                    = "US"
      + name                        = (known after apply)
      + project                     = (known after apply)
      + project_number              = (known after apply)
      + public_access_prevention    = (known after apply)
      + rpo                         = (known after apply)
      + self_link                   = (known after apply)
      + storage_class               = "STANDARD"
      + terraform_labels            = (known after apply)
      + uniform_bucket_level_access = (known after apply)
      + url                         = (known after apply)
    }

  # random_string.suffix will be created
  + resource "random_string" "suffix" {
      + id          = (known after apply)
      + length      = 64
      + lower       = false
      + min_lower   = 0
      + min_numeric = 0
      + min_special = 0
      + min_upper   = 0
      + number      = true
      + numeric     = true
      + result      = (known after apply)
      + special     = false
      + upper       = false
    }

Plan: 2 to add, 0 to change, 0 to destroy.
╷
│ Warning: Resource targeting is in effect
│ 
│ You are creating a plan
│ with the -target option,
│ which means that the
│ result of this plan may
│ not represent all of the
│ changes requested by the
│ current configuration.
│ 
│ The -target option is not
│ for routine use, and is
│ provided only for
│ exceptional situations
│ such as recovering from
│ errors or mistakes, or
│ when Terraform
│ specifically suggests to
│ use it as part of an
│ error message.
╵

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

random_string.suffix: Creating...
random_string.suffix: Creation complete after 0s [id=0808056569454331236561064499984914160477239699147199885142821831]
╷
│ Warning: Applied changes may be incomplete
│ 
│ The plan was created with
│ the -target option in
│ effect, so some changes
│ requested in the
│ configuration may have
│ been ignored and the
│ output values may not be
│ fully updated. Run the
│ following command to
│ verify that no other
│ changes are pending:
│     terraform plan
│ 
│ Note that the -target
│ option is not suitable
│ for routine use, and is
│ provided only for
│ exceptional situations
│ such as recovering from
│ errors or mistakes, or
│ when Terraform
│ specifically suggests to
│ use it as part of an
│ error message.
╵
╷
│ Error: "plan-0808056569454331236561064499984914160477239699147199885142821831" name value must contain 3-63 characters. Names containing dots can contain up to 222 characters, but each dot-separated component can be no longer than 63 characters
│ 
│   with google_storage_bucket.after_plan,
│   on test.tf line 10, in resource "google_storage_bucket" "after_plan":
│   10:   name     = "plan-${random_string.suffix.result}"

nbugden · 2024-04-12T19:36:41Z

however, since the validation is only on the create function for the current resource, it is possible to fix this error reducing the length of the random_string
since the validation is only on the create function for the current resource, it is possible to fix this error reducing the length of the random_string

@daniel-cit yes, updating the length of the suffix will fix the error as you mentioned. See output below.

Updating the Random Suffix Length

Updated Terraform Config

resource "random_string" "suffix" {
  length  = 34
  numeric = true
  lower   = false
  upper   = false
  special = false
}

Terraform Command

TF_CLI_CONFIG_FILE="$HOME/terraform/magic-modules-dev-override.tfrc" terraform apply -target='google_storage_bucket.after_plan'

Terraform Output

Terraform used the selected
providers to generate the
following execution plan.
Resource actions are
indicated with the
following symbols:
  + create
-/+ destroy and then create replacement

Terraform will perform the following actions:

  # google_storage_bucket.after_plan will be created
  + resource "google_storage_bucket" "after_plan" {
      + effective_labels            = (known after apply)
      + force_destroy               = false
      + id                          = (known after apply)
      + location                    = "US"
      + name                        = (known after apply)
      + project                     = "prj-c-scc-5xv2"
      + project_number              = (known after apply)
      + public_access_prevention    = (known after apply)
      + rpo                         = (known after apply)
      + self_link                   = (known after apply)
      + storage_class               = "STANDARD"
      + terraform_labels            = (known after apply)
      + uniform_bucket_level_access = true
      + url                         = (known after apply)
    }

  # random_string.suffix must be replaced
-/+ resource "random_string" "suffix" {
      ~ id          = "4459658187874002848905409602733573523031843089159360765646519166" -> (known after apply)
      ~ length      = 64 -> 34 # forces replacement
      ~ result      = "4459658187874002848905409602733573523031843089159360765646519166" -> (known after apply)
        # (9 unchanged attributes hidden)
    }

Plan: 2 to add, 0 to change, 1 to destroy.
╷
│ Warning: Resource targeting is in effect
│ 
│ You are creating a plan
│ with the -target option,
│ which means that the
│ result of this plan may
│ not represent all of the
│ changes requested by the
│ current configuration.
│ 
│ The -target option is not
│ for routine use, and is
│ provided only for
│ exceptional situations
│ such as recovering from
│ errors or mistakes, or
│ when Terraform
│ specifically suggests to
│ use it as part of an
│ error message.
╵

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

random_string.suffix: Destroying... [id=4459658187874002848905409602733573523031843089159360765646519166]
random_string.suffix: Destruction complete after 0s
random_string.suffix: Creating...
random_string.suffix: Creation complete after 0s [id=4905591332324978645088960881522615]
google_storage_bucket.after_plan: Creating...
google_storage_bucket.after_plan: Creation complete after 2s [id=plan-4905591332324978645088960881522615]
╷
│ Warning: Applied changes may be incomplete
│ 
│ The plan was created with
│ the -target option in
│ effect, so some changes
│ requested in the
│ configuration may have
│ been ignored and the
│ output values may not be
│ fully updated. Run the
│ following command to
│ verify that no other
│ changes are pending:
│     terraform plan
│ 
│ Note that the -target
│ option is not suitable
│ for routine use, and is
│ provided only for
│ exceptional situations
│ such as recovering from
│ errors or mistakes, or
│ when Terraform
│ specifically suggests to
│ use it as part of an
│ error message.
╵

Apply complete! Resources: 2 added, 0 changed, 1 destroyed.

nbugden · 2024-04-12T19:37:14Z

Sorry asking all these questions but I was affect by the initial version of the validation and I would like this change to be a smooth one.

@daniel-cit no problem! They are great questions. I agree it's better to have a smooth transition

modular-magician · 2024-04-12T20:37:55Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 94 insertions(+), 74 deletions(-))
google-beta provider: Diff ( 5 files changed, 94 insertions(+), 74 deletions(-))

modular-magician · 2024-04-12T21:14:18Z

Tests analytics

Total tests: 3576
Passed tests: 3207
Skipped tests: 368
Affected tests: 1

Click here to see the affected service packages

all service packages are affected

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccDataprocClusterIamPolicy

Get to know how VCR tests work

modular-magician · 2024-04-12T21:20:23Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{red}{\textsf{Errors occurred during RECORDING mode. Please fix them to complete your PR.}}$
View the build log or the debug log for each test

modular-magician · 2024-04-12T21:49:57Z

Tests analytics

Total tests: 3577
Passed tests: 3208
Skipped tests: 368
Affected tests: 1

Click here to see the affected service packages

all service packages are affected

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccDataSourceGoogleServiceAccountIdToken_impersonation

Get to know how VCR tests work

modular-magician · 2024-04-12T21:52:34Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataSourceGoogleServiceAccountIdToken_impersonation[Debug log]

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

modular-magician · 2024-04-12T21:57:20Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 5 files changed, 94 insertions(+), 74 deletions(-))
google-beta provider: Diff ( 5 files changed, 94 insertions(+), 74 deletions(-))

modular-magician · 2024-04-12T23:00:10Z

Tests analytics

Total tests: 3577
Passed tests: 3209
Skipped tests: 368
Affected tests: 0

Click here to see the affected service packages

all service packages are affected

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log

zli82016

LGTM. Thanks.

zli82016 · 2024-04-15T16:40:39Z

@daniel-cit, do you have any other comments?

daniel-cit · 2024-04-15T20:40:07Z

@daniel-cit, do you have any other comments?

@zli82016 No comments, LGTM

github-actions bot requested a review from zli82016 April 12, 2024 13:48

modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Apr 12, 2024

modular-magician added service/storage and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Apr 12, 2024

modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Apr 12, 2024

nbugden force-pushed the fix/gcs-validation branch from 9333713 to b3f3d9e Compare April 12, 2024 19:07

nbugden added 8 commits April 12, 2024 15:45

feat: test cases for ValidateGCSName

9976d25

feat: adds ValidateGCSName to verify package

6a6eca1

fix: use verify.ValidateGCSName in resource_storage_bucket

30dbf38

chore: clean up GCSNameLength validation

6709679

fix: has uppercase test

d7dc939

feat: gcs name test has underscore

824a21e

chore: remove duplicate validations from ChechGCSName

cc6220a

fix: GCSNameLengthSplit regex

87e70ad

Merge branch 'GoogleCloudPlatform:main' into fix/gcs-validation

30db557

modular-magician added awaiting-approval Pull requests that need reviewer's approval to run presubmit tests and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Apr 12, 2024

zli82016 approved these changes Apr 15, 2024

View reviewed changes

zli82016 merged commit 8e02735 into GoogleCloudPlatform:main Apr 15, 2024
14 checks passed

This was referenced Apr 15, 2024

fix: added gcs name validation hashicorp/terraform-provider-google-beta#7237

Merged

fix: added gcs name validation hashicorp/terraform-provider-google#17858

Merged

fix: added gcs name validation GoogleCloudPlatform/terraform-google-conversion#2160

Merged

nbugden deleted the fix/gcs-validation branch April 16, 2024 12:39

balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request Apr 19, 2024

fix: added gcs name validation (GoogleCloudPlatform#10426)

0074583

GoogleCloudPlatform deleted a comment Apr 22, 2024

balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request May 2, 2024

fix: added gcs name validation (GoogleCloudPlatform#10426)

b9cadd8

balanaguharsha pushed a commit to balanaguharsha/magic-modules that referenced this pull request May 2, 2024

fix: added gcs name validation (GoogleCloudPlatform#10426)

423f2c7

BBBmau pushed a commit to BBBmau/magic-modules that referenced this pull request May 8, 2024

fix: added gcs name validation (GoogleCloudPlatform#10426)

0d1597c

pawelJas pushed a commit to pawelJas/magic-modules that referenced this pull request May 16, 2024

fix: added gcs name validation (GoogleCloudPlatform#10426)

cac2e67

pengq-google pushed a commit to pengq-google/magic-modules that referenced this pull request May 21, 2024

fix: added gcs name validation (GoogleCloudPlatform#10426)

3204267

Cheriit pushed a commit to Cheriit/magic-modules that referenced this pull request Jun 4, 2024

fix: added gcs name validation (GoogleCloudPlatform#10426)

66cff69

pcostell pushed a commit to pcostell/magic-modules that referenced this pull request Jul 16, 2024

fix: added gcs name validation (GoogleCloudPlatform#10426)

14a59d7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: added gcs name validation #10426

fix: added gcs name validation #10426

nbugden commented Apr 12, 2024 •

edited by zli82016

Loading

github-actions bot commented Apr 12, 2024

zli82016 commented Apr 12, 2024

modular-magician commented Apr 12, 2024

daniel-cit commented Apr 12, 2024

nbugden commented Apr 12, 2024 •

edited

Loading

modular-magician commented Apr 12, 2024

nbugden commented Apr 12, 2024

daniel-cit commented Apr 12, 2024

daniel-cit commented Apr 12, 2024 •

edited

Loading

nbugden commented Apr 12, 2024

nbugden commented Apr 12, 2024

nbugden commented Apr 12, 2024

nbugden commented Apr 12, 2024 •

edited

Loading

modular-magician commented Apr 12, 2024

modular-magician commented Apr 12, 2024

modular-magician commented Apr 12, 2024

modular-magician commented Apr 12, 2024

modular-magician commented Apr 12, 2024

modular-magician commented Apr 12, 2024

modular-magician commented Apr 12, 2024

zli82016 left a comment

zli82016 commented Apr 15, 2024

daniel-cit commented Apr 15, 2024

fix: added gcs name validation #10426

fix: added gcs name validation #10426

Conversation

nbugden commented Apr 12, 2024 • edited by zli82016 Loading

github-actions bot commented Apr 12, 2024

zli82016 commented Apr 12, 2024

modular-magician commented Apr 12, 2024

Diff report

daniel-cit commented Apr 12, 2024

nbugden commented Apr 12, 2024 • edited Loading

modular-magician commented Apr 12, 2024

Tests analytics

nbugden commented Apr 12, 2024

daniel-cit commented Apr 12, 2024

daniel-cit commented Apr 12, 2024 • edited Loading

nbugden commented Apr 12, 2024

nbugden commented Apr 12, 2024

Terraform Config

Terraform Plan

Terraform Command

Output

Terraform Apply

Terraform Command

Terraform Output

nbugden commented Apr 12, 2024

Updating the Random Suffix Length

Updated Terraform Config

Terraform Command

Terraform Output

nbugden commented Apr 12, 2024 • edited Loading

modular-magician commented Apr 12, 2024

Diff report

modular-magician commented Apr 12, 2024

Tests analytics

Action taken

modular-magician commented Apr 12, 2024

modular-magician commented Apr 12, 2024

Tests analytics

Action taken

modular-magician commented Apr 12, 2024

modular-magician commented Apr 12, 2024

Diff report

modular-magician commented Apr 12, 2024

Tests analytics

zli82016 left a comment

Choose a reason for hiding this comment

zli82016 commented Apr 15, 2024

daniel-cit commented Apr 15, 2024

nbugden commented Apr 12, 2024 •

edited by zli82016

Loading

nbugden commented Apr 12, 2024 •

edited

Loading

daniel-cit commented Apr 12, 2024 •

edited

Loading

nbugden commented Apr 12, 2024 •

edited

Loading